New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add side navigation variant with icons on the left #2932
Conversation
Nice work, one suggestion: can we clip the border? @spencerbygraves what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartaz Approved with one additional mixin ehnancement proposal, feel free to ignore if you think it is overkill.
position: relative; | ||
} | ||
|
||
// nested 2nd level of navigation | ||
.p-side-navigation__item .p-side-navigation__item & { | ||
padding-left: 2 * $sph-inner; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the levels should be configurable when you invoke the mixin; many sites won't need 3 levels, and some might need 5; my proposal: @mixin p-side-navigation($depth: 3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels a bit overkill.
The design was discussed many times and it seems for now we barely have examples that would use all 3 level of navigation. If anything really needs more nesting we could add it later.
Also nesting is just 2 not very long rules (4 precisely if you count --icons
variant), so there is no much benefit in file size to reduce it to just one.
The complexity of adding configurable mixin that probably no one will use doesn't seem worth it. It doesn't seem like we do anything like this anywhere else.
While flexibility and ability to configure things is great it comes with cost of maintenance and ease of use.
I guess the biggest problem we are trying to solve with Vanilla is to keep our products consistent, so lets start with good default variants of components. If anything different is needed we can consider it on case by case basis.
@lyubomir-popov to draw attention to the text more than the icon? I've just noticed that in the design I made the hr #e5e5e5 rather than #cdcdcd, but I guess that would be a change to the hr everywhere? |
@spencerbygraves yes, to reenforce the text keyline.
then there are many lines that could be lighter:
|
@spencerbygraves For the border I used And what do you say about border width? Do we want to shorten it to align with text as per @lyubomir-popov suggestion? |
@bartaz @lyubomir-popov ok, let's keep the border as it is, it would be good to look at it's use as you mention above. Happy to try it aligned with the text. Would we need to bear this in mind for tables? Thanks |
No implications for tables, but could be a nice example to follow in the contextual menu pattern (if / when we add an example with icons). Added a new issue about border colours, to revisit when we have spare time. It is something we've discussed on a number of occasions so feels like we should make an official proposal: |
Done
Adds side navigation variant with icons on the left.
Fixes #2862
QA
./run
or demo